Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WD-8401 - Remove secrets #1704

Merged
merged 3 commits into from
Feb 8, 2024
Merged

WD-8401 - Remove secrets #1704

merged 3 commits into from
Feb 8, 2024

Conversation

huwshimi
Copy link
Contributor

@huwshimi huwshimi commented Feb 7, 2024

Done

  • Add a panel to remove secrets.

QA

  • Get the URI for a secret.
  • In a terminal add a few revisions to the secret: juju update-secret <URI> token=34ae35facd4 (you can run the same command a few times and each time a new revision will get created).
  • Open secrets tab.
  • Find the row for the secret you added the revisions to.
  • Open the actions menu on the right hand side and click "Remove secret".
  • In the panel either choose a revision or click the checkbox to remove them all.
  • Submit the form and confirm.
  • If all revisions were removed the secret should get removed from the list.

Details

https://warthogs.atlassian.net/browse/WD-8401

@webteam-app
Copy link

Demo starting at https://juju-dashboard-1704.demos.haus

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (e060dcb) 95.44% compared to head (02e6af7) 95.46%.

Files Patch % Lines
src/panels/RemoveSecretPanel/RemoveSecretPanel.tsx 95.65% 2 Missing ⚠️
src/panels/RemoveSecretPanel/Fields/Fields.tsx 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1704      +/-   ##
==========================================
+ Coverage   95.44%   95.46%   +0.01%     
==========================================
  Files         187      189       +2     
  Lines        5596     5686      +90     
  Branches     1616     1638      +22     
==========================================
+ Hits         5341     5428      +87     
- Misses        235      238       +3     
  Partials       20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@vladimir-cucu vladimir-cucu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works as expected locally (apart from the backend bug related to removing the last revision) 🚀

label={Label.REMOVE_ALL}
name="removeAll"
type="checkbox"
disabled={secret?.revisions.length === 1}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if maybe we could leave it always enabled? In the case when there is only 1 revision, we are still removing all the revisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it could be confusing if the checkbox was enabled as it's giving you a choice and the user might wonder if there will be a different outcome depending on which option they choose. I wonder if there is only one revision we should hide both fields and display some text instead? Maybe something like: "This secret has one revision (7) and will be completely removed."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea! If it is not that lengthy to implement, I think it might improve the user experience if done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@huwshimi
Copy link
Contributor Author

huwshimi commented Feb 8, 2024

Looks good and works as expected locally (apart from the backend bug related to removing the last revision) 🚀

I'm going to land this even though this bug exists, as this is not the only place this bug affects us, and once we have a solution we can update all the places (if it doesn't get rectified on the backend).

@huwshimi huwshimi merged commit 44271f1 into canonical:main Feb 8, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants